Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test date input for time slider and adjust input element #1095

Merged
merged 17 commits into from
Aug 8, 2024

Conversation

moontrip
Copy link
Contributor

This is related to #775. Tested controlled Brush by adding date input fields and it works fine: date picker is also added. In addition, considering the size of time slider, especially low height, a new prop, inputHeight, is added to adjust the height of the input element.

These are tested with storybook and video capture is made in the following. If this is alright then it will be applied to SAM.

TimeSlider.with.Date.input.and.picker.mp4

@moontrip moontrip requested a review from bobular May 15, 2024 12:25
@moontrip
Copy link
Contributor Author

@bobular applied to SAM and adjusted layout. It seems to work fine for me. Here are screenshots:

timeSlider datepicker01

timeSlider datepicker02

@moontrip
Copy link
Contributor Author

moontrip commented May 29, 2024

@bobular step buttons are added. Tested with storybook and SAM. Some notes are:

  • currently it is set to be 1 year as a step
  • right arrow increases range.end and left arrow decreases range.start for now
  • considered data min/max not to go over
  • also added a function parameter to change number of years for step (default = 1 year)
  • mouseover tooltip for each arrow is added

screenshots are:

  • enabled
    timeslider step buttons01

  • disabled
    timeslider step buttons02

Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @moontrip - this looks great!

I started looking at the components story and wasn't convinced - lots of things seemed broken or not yet implemented.

But then I tried the Map and it looks really cool. I love the layout.

I haven't had a look at the code yet, but I have worked on this ticket: #1103

The relevance is as follows:

  • the ➡️ button should move both start and end to the right by (end-start) - in other words move the selection across one "window"
  • obvs ⬅️ does similar
  • if there is not enough room to move the selection left or right then that button should be greyed out - it should not move partially into the remaining space.
  • it's essential that stepping back to a previous position creates exactly the same time filter (with no drift due to rounding, etc)

The last two requirements will enable stepping through the timeline to behave approximately like an animation, especially when all the requests have been cached.

My new ticket will add caching to visualization data requests also, so if the user has a floating plot on the screen, those will update quickly once all the responses have been cached.

@moontrip
Copy link
Contributor Author

@bobular Thank you for your reviews and comments. Yes some features are not implemented at storybook as there are some diff between storybook and actual one at SAM.

I will change the behavior of arrows next week as you suggested: I was also thinking of "window" :) As for the 4th item, I didn't notice a specific drift when playing with those buttons. Will see/test more when addressing your feedbacks.

One thing I am concerned is that for the example case I used (screenshots), the data range is over 100 years so the step of 1 year may be too small in such a case. Perhaps we can consider a sort of a dynamic step, e.g., data range/10? This may make user a confusion, though. On the other hand, 1 year may be okay as we also have date input/picker.

@bobular
Copy link
Member

bobular commented May 31, 2024

Hi @moontrip - is there a mock-up that says the step will be one year? I suggest it should be whatever the size of the selection is.

Thus after one click on ➡️ the window would move like this:

   [-------]
            [-------]

@moontrip
Copy link
Contributor Author

@bobular Aha I see. Thank you for your clarification. 👍

@moontrip
Copy link
Contributor Author

moontrip commented Jun 4, 2024

@bobular I addressed your feedbacks and committed them. Here is a short video capture, just in case: you can test it at your local more extensively :)

TimeSlider.step.buttons.mp4

@moontrip
Copy link
Contributor Author

moontrip commented Jul 1, 2024

@bobular You might have missed this review as you were hectic to deal with others. Can you please take a look at this when you are available?

@moontrip moontrip marked this pull request as ready for review July 1, 2024 13:09
@bobular bobular self-requested a review July 4, 2024 10:16
Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works really nicely and the EDA/MapVEu code looks good :-)

If possible please could you

  1. look into why the left/right arrows only get disabled after a "failed click" - is it possible to pre-empt this?
  2. When setting a year-based range (e.g. 2005-01-01 to 2006-01-01) would it be possible to handle leap-years so that the ranges always stay on Jan 1st? Even if the code is a bit ugly I think this would be worthwhile.

[setSelectedRange]
);

const handleArrowClick = useCallback(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is STORY code. I didn't understand how it worked but now I realise I should ignore it.

@moontrip
Copy link
Contributor Author

This works really nicely and the EDA/MapVEu code looks good :-)

If possible please could you

  1. look into why the left/right arrows only get disabled after a "failed click" - is it possible to pre-empt this?
  2. When setting a year-based range (e.g. 2005-01-01 to 2006-01-01) would it be possible to handle leap-years so that the ranges always stay on Jan 1st? Even if the code is a bit ugly I think this would be worthwhile.

@bobular Thank you for your reviews and suggestions 👍 Those suggestions seem to be quite tricky but I will see if I can address them 🤞

@moontrip
Copy link
Contributor Author

@bobular Your suggestions are addressed in some ways. For the first one, overall logic was changed to handle enabled/disabled, so it is now pre-emptive. For the second one that considers leap year, there are several edge cases to deal with, I suppose. Thus, what I did is to have a toggle button to move the range by year difference (please see a screenshot below) by ignoring month/date. For example when the toggle is on, a) 2007-01-01 to 2008-01-01 -> 2008-01-01 to 2009-01-01 (year 2008 is leap year so this becomes to 2008-12-31 if the toggle is off); b) 2007-01-01 to 2007-12-31 -> 2008-01-01 to 2008-12-31 (year difference is set to be 1 year at minimum)

perhaps the toggle label needs to be changed like Year range at least which needs some space arrangement for layout. I didn't do that yet because I want to see if this approach is alright.

By the way, vectorbase backend does not respond with map data but at least timeslider can be used for testing. It would be great if you can test it and share your thoughts.

time slider feedback01

@bobular
Copy link
Member

bobular commented Jul 30, 2024

Hi @moontrip - as I mentioned in Slack, I'm not too keen on the year-based-movement-toggle. One of the reasons is that we don't offer month- or week-based movement also (which I guess we could, but it would complicate the UI and implementation more than necessary IMO). Another is that it changes the stepping behaviour of the selected range in a way that's not very clear to users.

I had an idea while walking the dogs... It solves the issue of cleanly stepping through the years if the user has chosen Jan 1 as start and end dates (or any other identical start/end days of the year), without an extra toggle switch, and without explicitly checking to see if the user entered the same day-of-the-year as start and end dates.

Firstly we need a countLeapDays in date range function. Here's one that ChatGPT provided. It looks solid, but of course double-check.

function isLeapYear(year) {
    return (year % 4 === 0 && year % 100 !== 0) || (year % 400 === 0);
}

function countLeapDays(startDate, endDate) {
    const startYear = startDate.getFullYear();
    const endYear = endDate.getFullYear();
    let leapDayCount = 0;

    for (let year = startYear; year <= endYear; year++) {
        if (isLeapYear(year)) {
            const leapDay = new Date(year, 1, 29);
            if (leapDay >= startDate && leapDay <= endDate) {
                leapDayCount++;
            }
        }
    }

    return leapDayCount;
}

Then the main idea is to count the number of leap days in the current and next ranges and calculate the difference

  const deltaLeapDays = countLeapDays(newSelectedRange) - countLeapDays(selectedRange);

Then adjust the newSelectedRange.end by the number of days deficit/surplus.

Example:

Current range contains 2 leap days and has length 1500 days (e.g. 1 February 2024 to 11 March 2028). Next range contains 1 leap day. deltaLeapDays works out as -1. The next window length needs to be adjusted to have a length of 1499, achieved by subtracting one day from the end date. (Adjustments to the previous (left arrow) window need to be made to the newSelectedRange.start.)

There's an edge case where the current range is, for example, 2024-02-29 to 2024-02-29 (just one leap day). The new range would end up zero width, so we have to make sure the range is at least one day wide!

What do you reckon?

@moontrip
Copy link
Contributor Author

moontrip commented Jul 30, 2024

@bobular Thank you for your suggestion 👍 The logic looks reasonable so I will test it. Though the function is incorrect in part as the Date is not considered correctly :)

@moontrip
Copy link
Contributor Author

@bobular Ok it seems to work fine with your logic 👍 Here is a quick video capture: year 2008 is leap year

timeslider.leap.year.mp4

Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent. Thanks DK.

I'm not sure if the setDisableLeftArrow and setDisableRightArrow props for TimeSlider are needed? (And they are missing from useEffect dependencies in TimeSlider. They could also be combined into one callback: onBrushChange or onRangeChange.) Maybe this could be handled in TimeSliderQuickFilter in EDA instead - as part of setSelectedRange? It seems odd to refer to left and right arrows in a component that doesn't have them! Happy to have another look but you can fix and merge too if you like!

In general there seem to be some missing/extra deps that you could sort out please:

src/lib/map/analysis/TimeSliderQuickFilter.tsx
  Line 142:6:  React Hook useMemo has missing dependencies: 'extendedDisplayRange', 'studyId', and 'variableMetadata'. Either include them or remove the dependency array                                                                                                                                                                                                                react-hooks/exhaustive-deps
  Line 237:9:  The href attribute is required for an anchor to be keyboard accessible. Provide a valid, navigable address as the href value. If you cannot provide an href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid
  Line 295:5:  React Hook useCallback has an unnecessary dependency: 'extendedDisplayRange'. Either exclude it or remove the dependency array                                                                                                                                                                                                                                            react-hooks/exhaustive-deps

@moontrip
Copy link
Contributor Author

@bobular Thank you for your comments and suggestions. I will see if I can address those warnings: it is not always true to believe as it may sometimes cause issues (infinite loops for example).
By the way I am confused that the useEffect to deal with enabling/disabling arrows and those useState setter functions (setDisableLeftArrow and setDisableRightArrow) are all defined at TimeSliderQuickFilter where the arrow UI exists, not TimeSlider component? Did I miss or misunderstand something?

@bobular
Copy link
Member

bobular commented Jul 31, 2024

I'm not sure I follow but I think you could do something like this here

setSelectedRange={(selectedRange) =>
updateConfig({ ...config, selectedRange })
}

              setSelectedRange={(selectedRange) => {
                updateConfig({ ...config, selectedRange });
                setDisableLeftArrow(false);
                setDisableRightArrow(false);
              }}

@bobular
Copy link
Member

bobular commented Jul 31, 2024

Also, if required hook dependencies cause infinite loops that means the design should be revisited, in my opinion.

@moontrip
Copy link
Contributor Author

@bobular Thank you for your comments. I will check them out more carefully. 👍

@moontrip
Copy link
Contributor Author

moontrip commented Aug 6, 2024

@bobular I simplified codes which were originated from old logic. It would be great if you can check it out again. Thanks!

Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks cleaner and works great still.

@@ -234,7 +234,11 @@ export default function TimeSliderQuickFilter({
<p>
Expand the panel with the{' '}
<ChevronRight transform={'matrix(0,1,-1,0,0,0)'} /> tab or click{' '}
<a style={{ cursor: 'pointer' }} onClick={() => setMinimized(false)}>
<a
href="/#"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this something your IDE inserted? I think that happened once before. I don't think we want it...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobular Thank you for your reviews 👍 Yes it shows warning like "no href". The "/#" actually do nothing and it is one of recommended ways: otherwise, warning message recommends to use button element, but it is not the case for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Firefox the /# can show up bottom-left of the screen and I think it can end up in the address bar if clicked on. If you can remove that would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I didn't check Firefox. Okay then I will remove it and merge Thanks @bobular 👍

: endDate,
});
}, debounceRateMs),
[setSelectedRange, xAxisRange, debounceRateMs, xBrushScale]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting those missing deps!

);

// Cancel any pending onBrushChange requests when this component is unmounted
useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@moontrip moontrip merged commit 30425bd into main Aug 8, 2024
1 check passed
@moontrip moontrip deleted the timeslider-date-input branch August 8, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map - add forward backward step buttons to easy timeslider - add date pickers too (animation on the cheap)
2 participants